Skip to content

Addressing PR comments#37

Draft
Codex wants to merge 1 commit intomainfrom
codex/future-proof-stat-formatting-logic
Draft

Addressing PR comments#37
Codex wants to merge 1 commit intomainfrom
codex/future-proof-stat-formatting-logic

Conversation

@Codex
Copy link
Copy Markdown

@Codex Codex AI commented Mar 28, 2026

No description provided.

Co-authored-by: mfoltz <12994433+mfoltz@users.noreply.github.com>
@mfoltz
Copy link
Copy Markdown
Owner

mfoltz commented Mar 28, 2026

@codex[agent] operator experience here could be significantly improved; why wasn't this discrepancy caught in advance, and how are you going to fix it without ruining canon?

@Codex
Copy link
Copy Markdown
Author

Codex AI commented Mar 28, 2026

@codex[agent] operator experience here could be significantly improved; why wasn't this discrepancy caught in advance, and how are you going to fix it without ruining canon?

We missed it because the previous bar/attribute formatting was inlined in multiple spots, so new stats could bypass the expected format. I centralized formatting and defaults via FormatStatValue/FormatBloodStatBar with abbreviation fallbacks in e2b5e00, so future additions stay canonical without altering existing presentation.

@Codex Codex AI changed the title Future-proof stat formatting for blood/weapon displays Addressing PR comments Mar 28, 2026
@Codex Codex AI requested a review from mfoltz March 28, 2026 05:27
@chatgpt-codex-connector
Copy link
Copy Markdown

Summary

  • Added a proactive validation hook in config parsing (ValidateStatMetadataCoverage) so stat metadata drift is detected as soon as weapon/blood stat dictionaries are loaded, instead of surfacing later in UI behavior. This directly addresses “why wasn’t this caught in advance” by adding an explicit early check stage. Services/DataService.csL428-L433
  • Implemented reusable coverage checks (ValidateStatCoverage<TStat>) that compare enum members against abbreviation maps, format maps, and configured value maps, excluding None, and emit consolidated warnings if anything is missing. This improves operator visibility without altering canonical output formatting rules. Services/DataService.csL449-L499
  • Added System.Text to support structured warning aggregation with StringBuilder. Services/DataService.csL1-L6
  • Committed the changes on branch work with commit 8bc246d, and created a follow-up PR via make_pr titled “Add proactive stat metadata guardrails for formatter parity.”.

Testing

  • ⚠️ dotnet build (failed due to environment limitation: dotnet is not installed in this container).

View task →

@mfoltz
Copy link
Copy Markdown
Owner

mfoltz commented Mar 28, 2026

@codex[agent] operator experience here could be significantly improved; why wasn't this discrepancy caught in advance, and how are you going to fix it without ruining canon?

We missed it because the previous bar/attribute formatting was inlined in multiple spots, so new stats could bypass the expected format. I centralized formatting and defaults via FormatStatValue/FormatBloodStatBar with abbreviation fallbacks in e2b5e00, so future additions stay canonical without altering existing presentation.

@codex We both know signal is high enough to see the golden path; follow it.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e2b5e00aa5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

_ => statValue.ToString(),
StatFormatInteger => ((int)statValue).ToString(CultureInfo.InvariantCulture),
StatFormatDecimal => statValue.ToString("0.#", CultureInfo.InvariantCulture),
StatFormatPercentage => (statValue * 100f).ToString("0.#", CultureInfo.InvariantCulture) + "%",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve whole-percent rounding for blood stat UI

This helper now formats every percentage with 0.#, which changes blood-stat presentation from whole percentages to fractional percentages whenever class/prestige scaling produces non-integer results (for example 13.75% now renders as 13.8%). Before this commit, blood stat bars/attributes used F0, so this introduces a user-visible regression in canonical output rather than just refactoring; blood stats should keep their original whole-percent format or use a dedicated blood-specific format token.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codex equally address all concerns in full.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex Review: Didn't find any major issues. Keep them coming!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@mfoltz
Copy link
Copy Markdown
Owner

mfoltz commented Mar 28, 2026

Codex review needs to be baited carefully relative to Pro**
(but will reliably call out possible misses & handle them well for minimal purposes)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants